fix(qemu): detect and repair partial toolchain cache entries, preflight shared libraries - #1296
Conversation
…ht shared libraries Fixes #1266 — QEMU toolchain libslirp.so.0: cannot open shared object file (exit 127). Three defenses, stacked so each catches what the earlier misses: 1. **PackageBase::is_cached rejects installs without a sentinel.** staged_install already extracts-to-staging-then-atomic-rename and writes .install_complete, so a directory without one is a partial or legacy cache entry (CI cache restoration from an older fbuild, or a crash before the rename). Treating it as not-cached causes a clean re-extract rather than invoking a corrupt tree. 2. **QEMU install validation now verifies the bundled lib/ directory.** The Espressif tarball ships bin/qemu-system-xtensa and lib/libslirp.so.0 with rpath $ORIGIN/../lib. validate_install_* now asserts the lib/ directory exists alongside the binary, so a partial extract that somehow has the executable but lost the libs is caught at install-validation time (before the final rename). 3. **Preflight probe before returning the resolved binary.** On Linux, EspQemu::resolve_executable now runs qemu --version before handing the path back. Exit code 127 (dynamic linker failure) produces a diagnostic naming the missing library and the toolchain path, plus a rm -rf remediation line. The emulator runner also detects exit 127 and adds the same guidance as defense-in-depth. Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughQEMU toolchain validation now checks cache completion, bundled libraries, and Linux startup status. QEMU failures with exit code 127 report missing shared-library details and cached-toolchain recovery guidance. ChangesQEMU toolchain integrity
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Package cache
participant QEMU toolchain resolver
participant QEMU process handler
Package cache->>QEMU toolchain resolver: provide complete cached toolchain
QEMU toolchain resolver->>QEMU toolchain resolver: validate bundled lib/ directory
QEMU toolchain resolver->>QEMU process handler: run QEMU startup preflight
QEMU process handler-->>QEMU toolchain resolver: return startup status
QEMU process handler-->>Package cache: report exit 127 and cache-removal guidance
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fbuild-packages-fetch/src/lib.rs`:
- Around line 273-292: The staged_install flow must repair incomplete existing
installs instead of returning them. Before atomically renaming the staging
directory, write the install-complete sentinel and propagate any write failure
as an installation error; under the install lock, remove an existing install
directory when is_cached reports it incomplete, then reinstall it. Add a
regression test covering staged_install with a pre-existing directory missing
the sentinel.
In `@crates/fbuild-toolchain/src/toolchain/esp_qemu.rs`:
- Around line 127-131: Update the cached-install branch in EspQemu’s resolver
around is_installed to call qemu_validate_bundled_libs before accepting the QEMU
path, including the macOS path that currently skips preflight. When validation
fails, route the result through the existing cache-repair flow instead of
returning the broken cached path.
- Around line 240-255: The QEMU diagnostics derive unsafe cache deletion paths
from the executable location. In
crates/fbuild-toolchain/src/toolchain/esp_qemu.rs lines 240-255, carry the
fbuild-owned cache root into the diagnostic and emit a deletion command only for
that exact managed cache entry; omit deletion guidance for external paths
resolved through environment variables or PATH. In
crates/fbuild-daemon/src/handlers/emulator/shared.rs lines 361-370, remove
executable-parent cleanup guidance, retain the executable path, and show cache
cleanup only when a verified fbuild cache root is available.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ef000bb-0bb8-48a2-9151-d878b9972699
📒 Files selected for processing (3)
crates/fbuild-daemon/src/handlers/emulator/shared.rscrates/fbuild-packages-fetch/src/lib.rscrates/fbuild-toolchain/src/toolchain/esp_qemu.rs
| /// | ||
| /// Requires both the install directory AND the `.install_complete` sentinel | ||
| /// to be present. A directory without the sentinel is an incomplete or | ||
| /// partial install (e.g. a CI cache restored from a crashed job before the | ||
| /// atomic rename was committed, or an extract interrupted mid-flight). The | ||
| /// caller should treat this as "not installed" so the package is | ||
| /// re-extracted rather than invoked with a corrupt tree. | ||
| /// | ||
| /// On a cache hit, bumps the LRU timestamp in the DiskCache index. | ||
| pub fn is_cached(&self) -> bool { | ||
| let path = self.install_path(); | ||
| let cached = path.exists() && path.is_dir(); | ||
| if cached { | ||
| self.touch_disk_cache(); | ||
| if !path.exists() || !path.is_dir() { | ||
| return false; | ||
| } | ||
| cached | ||
| let sentinel = disk_cache::paths::install_complete_sentinel(&path); | ||
| if !sentinel.exists() { | ||
| return false; | ||
| } | ||
| self.touch_disk_cache(); | ||
| true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Repair incomplete cache directories before returning them.
At Line 344, staged_install returns an existing install directory before it validates or replaces it. The new predicate reports a missing sentinel as a cache miss, but the retry returns the same incomplete directory.
Write the sentinel in staging before the atomic rename. Treat a sentinel write failure as an install failure. Under the install lock, remove and reinstall an existing directory that does not meet the completeness requirement. Add a regression test that calls staged_install against an existing directory without the sentinel.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-packages-fetch/src/lib.rs` around lines 273 - 292, The
staged_install flow must repair incomplete existing installs instead of
returning them. Before atomically renaming the staging directory, write the
install-complete sentinel and propagate any write failure as an installation
error; under the install lock, remove an existing install directory when
is_cached reports it incomplete, then reinstall it. Add a regression test
covering staged_install with a pre-existing directory missing the sentinel.
Source: Coding guidelines
| } else if self.is_installed() { | ||
| let path = find_qemu_binary(&self.base.install_path(), self.arch)?; | ||
| hydrate_windows_runtime(&path)?; | ||
| validate_windows_runtime(&path)?; | ||
| return Ok(path); | ||
| } | ||
|
|
||
| if let Some(path) = find_existing_idf_qemu(self.arch) { | ||
| path |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Validate bundled libraries before accepting a cached QEMU installation.
EspQemu::is_installed checks for a sentinel and executable, but it does not call qemu_validate_bundled_libs. A cache restore can preserve those two items while omitting lib/.
On macOS, Lines 215-219 skip the preflight. The resolver then returns a broken cached QEMU path. Include bundled-library validation in the cached-install path, and route a failed validation through cache repair.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-toolchain/src/toolchain/esp_qemu.rs` around lines 127 - 131,
Update the cached-install branch in EspQemu’s resolver around is_installed to
call qemu_validate_bundled_libs before accepting the QEMU path, including the
macOS path that currently skips preflight. When validation fails, route the
result through the existing cache-repair flow instead of returning the broken
cached path.
| Err(FbuildError::PackageError(format!( | ||
| "QEMU at {} cannot start: a required shared library is missing.\n\ | ||
| {}\n\ | ||
| The cached QEMU toolchain appears incomplete or corrupt.\n\ | ||
| To fix, delete the cached toolchain and retry:\n rm -rf {}", | ||
| qemu_binary.display(), | ||
| missing.as_deref().unwrap_or(&format!( | ||
| "The dynamic linker reported: {}", | ||
| stderr.trim() | ||
| )), | ||
| qemu_binary | ||
| .parent() | ||
| .and_then(|p| p.parent()) | ||
| .unwrap_or(qemu_binary.parent().unwrap_or(qemu_binary)) | ||
| .display(), | ||
| ))) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not derive cache deletion commands from the executable path.
QEMU can resolve from an environment variable or PATH, not only from the fbuild cache. For /usr/bin/qemu-system-xtensa, the toolchain diagnostic suggests rm -rf /usr, and the daemon diagnostic suggests deletion of /usr/bin.
crates/fbuild-toolchain/src/toolchain/esp_qemu.rs#L240-L255: Carry the fbuild-owned cache root into the diagnostic. Only show a deletion command for that exact managed cache entry. Omit deletion guidance for external QEMU paths.crates/fbuild-daemon/src/handlers/emulator/shared.rs#L361-L370: Remove executable-parent cleanup guidance. Report the executable path, but show cache cleanup only when a verified fbuild cache root is available.
📍 Affects 2 files
crates/fbuild-toolchain/src/toolchain/esp_qemu.rs#L240-L255(this comment)crates/fbuild-daemon/src/handlers/emulator/shared.rs#L361-L370
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-toolchain/src/toolchain/esp_qemu.rs` around lines 240 - 255,
The QEMU diagnostics derive unsafe cache deletion paths from the executable
location. In crates/fbuild-toolchain/src/toolchain/esp_qemu.rs lines 240-255,
carry the fbuild-owned cache root into the diagnostic and emit a deletion
command only for that exact managed cache entry; omit deletion guidance for
external paths resolved through environment variables or PATH. In
crates/fbuild-daemon/src/handlers/emulator/shared.rs lines 361-370, remove
executable-parent cleanup guidance, retain the executable path, and show cache
cleanup only when a verified fbuild cache root is available.
The preflight used bare std::process::Command::new which (a) flashes a console window on Windows daemon-hosted emulator runs and (b) trips the allow-direct-spawn lint gate. Switch to fbuild_core::subprocess:: run_command_blocking which routes through containment on all platforms. Co-Authored-By: Claude <noreply@anthropic.com>
The return Ok(()); in the #[cfg(not(target_os = "linux"))] branch of preflight_qemu_binary is the last statement in the function on macOS, triggering clippy's needless_return lint (denied by -D warnings). Replace with a plain tail expression. Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Fixes #1266 — QEMU toolchain
qemu-system-xtensafails withlibslirp.so.0: cannot open shared object file(exit 127).Problem
When the cached Espressif QEMU toolchain is partial or corrupt (e.g. a CI cache restored from a crashed job before the atomic rename was committed, or an extract interrupted mid-flight),
is_installed()reported true (the binary exists) but the binary couldn't find its bundled shared libraries at runtime. The emulator runner then reported a bare "exited with code 127" with no toolchain-path context.Three defenses, stacked
PackageBase::is_cachednow requires the.install_completesentinel.staged_installalready extracts-to-staging-then-atomic-rename and writes the sentinel, so a directory without one is a partial or legacy cache entry. Treating it as not-cached triggers a clean re-extract.QEMU install validation verifies the bundled
lib/directory. The Espressif tarball shipsbin/qemu-system-xtensa+lib/libslirp.so.0with rpath$ORIGIN/../lib.validate_install_xtensa/validate_install_riscv32now assertlib/exists alongside the binary.Preflight probe before returning the resolved binary. On Linux,
EspQemu::resolve_executablerunsqemu --versionbefore returning the path. Exit code 127 produces a diagnostic naming the missing library and the toolchain path, plus arm -rfremediation. The emulator runner also detects exit 127 for defense-in-depth.Tests
is_cached_returns_false_when_sentinel_is_missing— verifies the sentinel gatebundled_libs_ok_standard_layout_bin_and_lib,bundled_libs_ok_alt_layout_top_level_with_lib,bundled_libs_missing_lib_dir_is_error,bundled_libs_binary_at_root_no_lib_dir_is_error— validate bundled lib/ detectionpreflight_ok_when_binary_runs_version_successfully,preflight_linux_detects_missing_shared_library_exit_127— preflight probefbuild-packages-fetch(131 tests) andfbuild-toolchain(131 tests) suites passAcceptance criteria
fbuild test-emuinvocations sharing one cold QEMU toolchain cache key both start the emulator successfully — the install-lock + atomic rename already handle this; this PR adds the sentinel gate so a restored partial cache is rejected.libsdl2-2.0-0install step fromqemu_template.yml— the preflight catches missing system deps with an actionable message; the sentinel gate prevents partial extractions.🤖 Generated with Claude Code
Summary by CodeRabbit